Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Don't remove spaces when printing a new bottom line with a background color #5550

Merged
7 commits merged into from
Apr 30, 2020
Merged

Don't remove spaces when printing a new bottom line with a background color #5550

7 commits merged into from
Apr 30, 2020

Conversation

zadjii-msft
Copy link
Member

@zadjii-msft zadjii-msft commented Apr 24, 2020 •
edited by DHowett-MSFT
Loading

Turns out we're still being a bit too aggressive when removing spaces.
If there are spaces at the end of the first run painted to a bottom
line, and the bottom line was a different color than the previous,
then we can't trim those spaces off the string. We still need to emit
those to make sure the terminal has colored spaces in it as well.

References

  • there's like 80 PRs in the last month for this function

PR Checklist

Validation Steps

  • ran the tests
  • checked that vtpipeterm still worked
  • Checked that the bug was fixed in the Terminal

@zadjii-msft zadjii-msft added Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Product-Conpty For console issues specifically related to conpty labels Apr 24, 2020
DHowett-MSFT
DHowett-MSFT reviewed Apr 24, 2020
View reviewed changes
Copy link
Contributor

@DHowett-MSFT DHowett-MSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand why we're hitting the "optimize spaces" case between two other characters.

Also, do you think it would be possible to save the whole TextAttribute and use the HasIdenticalVisualRepresentationForBlankSpace function? It takes care of cases like global and local inverse (which James pointed out) which I think could be a ... problem .. here

nevermind, it won't be a problem here because we're using the COLORREFs and we've lost text attribute info by this point (#2661)

Caching more state always scares me :)

@zadjii-msft
Copy link
Member Author

So in this case, the row we're printing is separated into two runs:

  • "    #         "
    • This run is in green-on-blue
  • "#             "...
    • This run is in the default attrs

It's that first run's 9 spaces that we tried to optimize with the ECH.

I could certainly see us needing to cache more info, but maybe that's just something we should handle in #2661 when we have all the info in one nice blob

DHowett-MSFT
DHowett-MSFT approved these changes Apr 29, 2020
View reviewed changes
Copy link
Contributor

@DHowett-MSFT DHowett-MSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love it, but it works and that's more important than ideology.

@zadjii-msft zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Apr 29, 2020
@ghost ghost requested review from miniksa, carlos-zamora and leonMSFT April 29, 2020 20:19
miniksa
miniksa approved these changes Apr 29, 2020
View reviewed changes
Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This spaghetti is killing me, but if it works, I have to agree with Dustin and just let it go. At least there are more tests than we had two months ago before this started so we might have some hope of making this less gross in the future.

@DHowett-MSFT DHowett-MSFT added the AutoMerge Marked for automatic merge by the bot when requirements are met label Apr 29, 2020
@ghost ghost added the Issue-Bug It either shouldn't be doing this or needs an investigation. label Apr 29, 2020
@ghost
Copy link

ghost commented Apr 29, 2020

Hello @DHowett-MSFT!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@DHowett-MSFT
Copy link
Contributor

Wow -- merging master actually broke the test (???) Sorry :(

@ghost ghost merged commit 4286877 into master Apr 30, 2020
@ghost ghost deleted the dev/migrie/b/5502-background-colors-after-circling branch April 30, 2020 15:01
@ghost
Copy link

ghost commented May 5, 2020

🎉Windows Terminal Release Candidate v0.11.1251.0 (1.0rc1) has been released which incorporates this pull request.:tada:

Handy links:

@ghost ghost mentioned this pull request May 5, 2020
Closed
@zadjii-msft zadjii-msft mentioned this pull request May 12, 2020
Open
31 tasks
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miniksa miniksa miniksa approved these changes

@DHowett-MSFT DHowett-MSFT DHowett-MSFT approved these changes

@carlos-zamora carlos-zamora Awaiting requested review from carlos-zamora

@leonMSFT leonMSFT Awaiting requested review from leonMSFT

Assignees
No one assigned
Labels
Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Second It's a PR that needs another sign-off Product-Conpty For console issues specifically related to conpty
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

New 0.11 VT colors bug, background color isn't applied to " " (space) cells
4 participants
@zadjii-msft @DHowett-MSFT @miniksa @DHowett